stack 3/7: carry six contributor bug fixes with authorship intact - #953
Conversation
A tool_result whose content is a document block translated to an empty tool output, so routed models treated the attachment as a tool that returned nothing. Surface the same "[document: title]" marker the user-message path already emits. (cherry picked from commit 3cdf190)
Gemini usually drops the malformed call upstream, so the final chunk carries only the finishReason and the started-calls guard never fired. The turn surfaced as a clean empty completion instead of an error. MAX_TOKENS with no started call keeps its plain token-limit stop. (cherry picked from commit 6eb5903)
The streaming translator folded every summary part into one thinking block with no separator, so multi-part summaries rendered as run-on text. The JSON path already joins parts with a blank line; the stream now emits the same separator at part boundaries. (cherry picked from commit b23cc1f)
The native branch buffers the upstream compact JSON and returns it without inspecting the body, so the request log row lands with no usage. Lift usage and response metadata from the buffered body the same way the routed branch gets it through handleResponses. (cherry picked from commit 2ae3b5f)
(cherry picked from commit 88d60c3)
Kiro never returns plaintext reasoning for its Sol-family models. Its `reasoningContentEvent` carries a KMS-encrypted `redactedContent` blob, and `gpt-5.6-sol`'s `additionalModelRequestFieldsSchema` accepts only `reasoning.effort` — there is no display or summary opt-in. Kiro's own CLI replays that blob on the matching `assistantResponseMessage.reasoningContent` to preserve model reasoning across turns. The adapter read only `reasoningContentEvent.text`, which is absent on this wire, so the blob was dropped and never replayed. Every turn therefore restarted without the previous turn's reasoning. - kiro-events: parse `redactedContent`; add the previously unhandled `contextUsageEvent` (Kiro reports context pressure there, not in `metadataEvent`, which carries only `stopReason`). - Carry the blob through the existing `ocxr1:` envelope as `krc` on an envelope-only reasoning item, so it round-trips while staying invisible in the app — the same contract the hidden-thinking path already uses. - Pair it backwards: Kiro emits the event at the END of a turn, after content and tool calls, so a krc-only item belongs to the assistant turn that already closed. Folding it forward would attach turn N's blob to turn N+1. With no assistant turn to own it, the blob is dropped rather than mis-paired. - Replay it on `assistantResponseMessage.reasoningContent`. Verified against kiro-cli 2.14.1 and 2.16.0 request/response captures. (cherry picked from commit cbb5d21)
CodeRabbit caught a real defect that made the round-trip a no-op in the streaming path. Kiro sends its reasoning blob at the END of a turn, while the assistant message is still open. Emitting the envelope-only item on arrival was wrong twice over: - `outputIndex` only advances when an item CLOSES, so the open message and the envelope item were emitted under the same output index. - The envelope landed BEFORE the assistant message, and the parser pairs a krc-only item backwards, so it found no preceding assistant turn and dropped the blob as orphaned — silently defeating the fix. Both paths now stash the blob and flush it after every open item has closed: after the closes in the streaming `done` case, and after the trailing flushes in the batch path. Message phase inference is untouched, so a Kiro final answer is still classified `final_answer` rather than being force-closed as commentary. The batch path also released `bytesOf(encrypted)` through `pushOutput` without ever retaining it. It now charges the blob when stashing and lets `pushOutput` release that retained allocation, so the translator budget balances. Adds tests/kiro-reasoning-roundtrip.test.ts, which bridges adapter events and re-parses the emitted items the way Codex replays history — the end-to-end coverage the original tests lacked. Three of its five cases fail against the previous commit. Scope: verified that gpt-5.6-terra and gpt-5.6-luna return `redactedContent` exactly like gpt-5.6-sol, so the whole GPT-5.6 family was affected. Handling keys off the wire field, not the model id. (cherry picked from commit bd13d48)
…blob The batch budget assertion was vacuous twice over. It passed the budget in the wrong argument position, so `buildResponseJSON` built its own internal budget and the snapshot under test was never written to (`highWaterBytes: 0`). Even wired correctly, asserting `currentBytes >= 0` could not distinguish a leaked raw blob from the finalized items that legitimately stay retained. It now uses a 4 KB blob and asserts `currentBytes` equals EXACTLY the finalized output items' bytes — a still-retained raw blob shows up as ~4 KB of excess, and releasing bytes that were never charged shows up as a shortfall. A separate `highWaterBytes` assertion proves the blob was charged while held rather than never accounted for. Four of the file's five cases now fail against cbb5d21. Also corrects the metadataEvent description: every capture put the context percentage in `contextUsageEvent`, but the parser still accepts a finite `contextUsagePercentage` (and `tokenUsage`) from `metadataEvent` as a fallback, so documenting it as impossible was wrong. (cherry picked from commit 8610630)
📝 WalkthroughWalkthroughChangesTerminal-aware SSE transport
Kiro reasoning round-trip
Provider and adapter behavior
Response usage inspection
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
Stack navigation
Review and merge bottom-up. Each PR targets the preceding stack branch, so its Files changed view contains only that layer. The layers touch disjoint files — Carried in #953, with authorship preserved: #939, #942, #943, #944, #945, #948. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9664abdd18
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const clientBody = process.platform === "win32" && !needsClientRewrite | ||
| ? nativeBody | ||
| : relaySseWithFailedTail(rewrittenBody, upstream, reason => clientGone.abort(reason)); | ||
| const clientBody = relaySseWithFailedTail(rewrittenBody, upstream, reason => clientGone.abort(reason)); |
There was a problem hiding this comment.
Preserve the native Windows tee client path
On Windows with the default streamMode: "auto" or explicit legacy-tee, no-rewrite traffic does not enter the eager branch because MIN_FIXED_BUN_VERSION is still null; it therefore reaches this unconditional relaySseWithFailedTail JS pull wrapper. This recreates the Bun#32111 path that the removed nativeBody branch avoided, so the bundled known-bad runtime can crash or lose terminal frames. Keep the native client branch for Windows decisions where useEagerRelay is false, or only use a JS relay once the runtime is proven fixed.
AGENTS.md reference: AGENTS.md:L206-L208
Useful? React with 👍 / 👎.
| return { | ||
| feed(chunk) { | ||
| if (disposed || terminal) return new Uint8Array(0); | ||
| buffer += decoder!.decode(chunk, { stream: true }); |
There was a problem hiding this comment.
Bound incomplete frames in the terminal relay
When an upstream sends a very large or never-terminated SSE event, every chunk is appended to buffer until a blank-line delimiter arrives, with no byte cap or timeout. This new boundary is now on passthrough response paths, so a stuck or hostile compatible gateway can grow proxy memory without limit and withhold all of that event from the client. Apply a bounded frame policy and fail closed or safely fall back when the cap is exceeded.
AGENTS.md reference: src/AGENTS.md:L17-L17
Useful? React with 👍 / 👎.
Stack navigation
Review and merge bottom-up. Each PR targets the preceding stack branch, so its Files changed view contains only that layer. The layers touch disjoint files — #954 needs human security review per Carried in #953, with authorship preserved: #939, #942, #943, #944, #945, #948. |
Source PRs are now closed as carriedAll six have been closed and point here. Each was verified byte-identical before closing —
Authorship is preserved by This changes the failure mode, so it is worth stating plainly. My earlier comments said the sources would stay open until this merged. They are now closed while this PR is still under review, which means if this stack does not land, those six fixes are no longer sitting in the queue on their own. Reopening any of them is one click and their branches are untouched — but the responsibility for that now sits with this PR rather than with them. |
Stack navigation — 7 layers, review and merge bottom-up
Each layer targets the branch below it, so its diff only makes sense on that base — Note for the merge sequence: retargeting a child after its parent merges emits an |
# Conflicts: # tests/responses-compaction-routing.test.ts
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/bridge.ts`:
- Around line 460-482: Update every terminal streaming path in the bridge,
including incomplete, error, adapter-EOF, and stall-timeout handling, to call
flushKiroRedactedReasoning() after closing open output items and before emitting
the terminal Responses event. Preserve the existing done behavior, ensure
pending data is released and emitted on abnormal termination, and add regression
coverage for incomplete and error after receiving a Kiro blob.
In `@src/server/index.ts`:
- Around line 330-332: Update the relay-policy comment near the gated-shape
logic to distinguish Windows rewrite traffic, which always uses eager relay,
from Windows no-rewrite traffic, which follows selectEagerPath and may remain
tee for legacy-tee or known-bad-runtime auto decisions.
In `@src/server/relay.ts`:
- Around line 111-153: Bound the incomplete-frame accumulation in
createSseTerminalOutputBoundary, including the decoder state used by feed, so
delimiter-free input cannot grow buffer indefinitely. When the byte cap is
exceeded, terminate processing with a bounded response.failed sequence and
release decoder state; ensure both relaySseWithFailedTail and
relaySseEagerBounded use this behavior. Add coverage for an open stream that
continually emits data without an SSE delimiter.
In `@src/server/responses/core.ts`:
- Around line 2035-2041: Update the response stream composition around
relaySseWithPayloadRewrite and relaySseWithFailedTail so nativeBody is first
passed through relaySseWithFailedTail, then the terminal-bounded stream is
passed to relaySseWithPayloadRewrite when payload rewrites exist. Preserve
nativeBody directly when no rewrites are configured, and add a regression test
covering a payload rewrite with a coalesced response.completed frame followed by
an oversized tail.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: ca082d0b-ee05-46e5-a515-fa95177a8075
📒 Files selected for processing (28)
src/adapters/google-truncation.tssrc/adapters/google.tssrc/adapters/kiro-events.tssrc/adapters/kiro.tssrc/bridge.tssrc/claude/inbound.tssrc/claude/outbound.tssrc/lib/bun-stream-caps.tssrc/responses/parser.tssrc/responses/reasoning-envelope.tssrc/server/index.tssrc/server/relay-eager.tssrc/server/relay.tssrc/server/responses/compact.tssrc/server/responses/core.tssrc/types.tsstructure/04_transports-and-sidecars.mdtests/anthropic-thinking-signature.test.tstests/claude-inbound.test.tstests/claude-outbound.test.tstests/google-vertex-stream.test.tstests/kiro-adapter.test.tstests/kiro-reasoning-roundtrip.test.tstests/kiro-stream.test.tstests/passthrough-abort.test.tstests/relay-eager.test.tstests/responses-compaction-routing.test.tstests/sse-failed-tail.test.ts
| // Kiro reasoning round-trip. Kiro sends its encrypted blob at the END of a turn, while the | ||
| // assistant message is still open, so this CANNOT emit on arrival: the open message still | ||
| // owns `outputIndex` (it only advances on close), and an item emitted here would both reuse | ||
| // that index and land BEFORE the message — where the parser's backwards pairing drops it as | ||
| // orphaned. Stash it and flush after `done` has closed every open item instead. | ||
| let pendingKiroRedacted: string | undefined; | ||
| let pendingKiroRedactedBytes = 0; | ||
| const flushKiroRedactedReasoning = () => { | ||
| if (!pendingKiroRedacted) return; | ||
| const previousBytes = pendingKiroRedactedBytes; | ||
| const encrypted = encodeReasoningEnvelope({ krc: pendingKiroRedacted }); | ||
| const reservation = budget?.reserveTransient(bytesOf(encrypted), { kind: "reasoning" }); | ||
| pendingKiroRedacted = undefined; | ||
| pendingKiroRedactedBytes = 0; | ||
| reservation?.commitRetained(); | ||
| budget?.releaseRetained(previousBytes, { kind: "reasoning" }); | ||
| const itemId = `rs_${uuid()}`; | ||
| const item = { type: "reasoning", id: itemId, summary: [] as never[], encrypted_content: encrypted }; | ||
| emit("response.output_item.added", { output_index: outputIndex, item }); | ||
| emit("response.output_item.done", { output_index: outputIndex, item }); | ||
| retainFinishedItem(item as OutputItem, bytesOf(encrypted), "reasoning"); | ||
| outputIndex++; | ||
| }; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Flush the Kiro blob on every terminal path.
Lines 1100-1102 flush pendingKiroRedacted only for done. The incomplete, error, adapter-EOF, and stall-timeout paths do not flush or release it. A stream that receives kiro_redacted_reasoning and then terminates abnormally loses the replay state. It also leaves the retained blob charged in an external TranslatorBudget.
Call flushKiroRedactedReasoning() after closing open output items in every terminal path, before emitting the terminal Responses event. Add regression tests for incomplete and error after a Kiro blob. This aligns streaming behavior with batch finalization at Lines 1717-1725.
Also applies to: 921-926, 1100-1102
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/bridge.ts` around lines 460 - 482, Update every terminal streaming path
in the bridge, including incomplete, error, adapter-EOF, and stall-timeout
handling, to call flushKiroRedactedReasoning() after closing open output items
and before emitting the terminal Responses event. Preserve the existing done
behavior, ensure pending data is released and emitted on abnormal termination,
and add regression coverage for incomplete and error after receiving a Kiro
blob.
| // #314 gated shape: win32 always uses the terminal-aware eager relay so a keep-alive | ||
| // upstream cannot hold Codex open after response.completed; darwin no-rewrite traffic | ||
| // requires explicit config-eager opt-in (`auto` always stays tee on darwin). |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the Windows relay-policy comment.
Line 330 says Windows always uses the eager relay. In src/server/responses/core.ts, Windows no-rewrite traffic uses eager relay only when selectEagerPath permits it. legacy-tee and a known-bad-runtime auto decision remain on tee.
State that Windows rewrite traffic always uses eager relay, while Windows no-rewrite traffic follows selectEagerPath.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/server/index.ts` around lines 330 - 332, Update the relay-policy comment
near the gated-shape logic to distinguish Windows rewrite traffic, which always
uses eager relay, from Windows no-rewrite traffic, which follows selectEagerPath
and may remain tee for legacy-tee or known-bad-runtime auto decisions.
| export function createSseTerminalOutputBoundary(): SseTerminalOutputBoundary { | ||
| let decoder: TextDecoder | null = new TextDecoder(); | ||
| const encoder = new TextEncoder(); | ||
| let buffer = ""; | ||
| let terminal = false; | ||
| let done = false; | ||
| let disposed = false; | ||
|
|
||
| const process = (flush: boolean): Uint8Array => { | ||
| if (disposed || terminal) return new Uint8Array(0); | ||
| let output = ""; | ||
| let responsesTerminal = false; | ||
| for (;;) { | ||
| const next = nextSseBlock(buffer); | ||
| if (!next) break; | ||
| buffer = next.rest; | ||
| const payload = sseDataPayload(next.block); | ||
| if (!responsesTerminal) output += next.block + next.delimiter; | ||
| if (payload === "[DONE]") { | ||
| done = true; | ||
| if (responsesTerminal) output += next.block + next.delimiter; | ||
| continue; | ||
| } | ||
| if (!responsesTerminal && payload && terminalStatusFromSsePayload(payload)) { | ||
| responsesTerminal = true; | ||
| } | ||
| } | ||
| if (responsesTerminal) { | ||
| terminal = true; | ||
| buffer = ""; | ||
| } | ||
| if (flush && !terminal && buffer.length > 0) { | ||
| output += buffer; | ||
| buffer = ""; | ||
| } | ||
| return encoder.encode(output); | ||
| }; | ||
|
|
||
| return { | ||
| feed(chunk) { | ||
| if (disposed || terminal) return new Uint8Array(0); | ||
| buffer += decoder!.decode(chunk, { stream: true }); | ||
| return process(false); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Bound the incomplete SSE frame buffer.
Line 152 appends every undecimited fragment to buffer. A gateway can keep a malformed SSE frame open indefinitely. This retains unbounded decoded text in both relaySseWithFailedTail and relaySseEagerBounded.
Add a byte cap for the incomplete frame. If the cap is exceeded, fail the relay with a bounded response.failed sequence and release the decoder state. Add tests for an open stream that continually sends data without an SSE delimiter.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/server/relay.ts` around lines 111 - 153, Bound the incomplete-frame
accumulation in createSseTerminalOutputBoundary, including the decoder state
used by feed, so delimiter-free input cannot grow buffer indefinitely. When the
byte cap is exceeded, terminate processing with a bounded response.failed
sequence and release decoder state; ensure both relaySseWithFailedTail and
relaySseEagerBounded use this behavior. Add coverage for an open stream that
continually emits data without an SSE delimiter.
Source: Path instructions
| // Windows was handled by the eager terminal-aware branch above. Remaining | ||
| // tee traffic can use the JS relay to close on a protocol terminal and to | ||
| // convert a mid-stream reset into a clean response.failed event. | ||
| const rewrittenBody = payloadRewrites.length > 0 | ||
| ? relaySseWithPayloadRewrite(nativeBody, composeSsePayloadRewrites(...payloadRewrites), translatorBudget) | ||
| : nativeBody; | ||
| const clientBody = process.platform === "win32" && !needsClientRewrite | ||
| ? nativeBody | ||
| : relaySseWithFailedTail(rewrittenBody, upstream, reason => clientGone.abort(reason)); | ||
| const clientBody = relaySseWithFailedTail(rewrittenBody, upstream, reason => clientGone.abort(reason)); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Apply terminal filtering before payload rewriting.
relaySseWithPayloadRewrite receives and budgets the full upstream chunk before relaySseWithFailedTail sees response.completed. If one chunk contains a terminal frame plus a large trailing frame, the rewrite buffer can reject the trailing data before the client receives the valid terminal frame.
Wrap nativeBody with relaySseWithFailedTail first. Then apply relaySseWithPayloadRewrite to that terminal-bounded stream. Add a regression test with a payload rewrite, a coalesced terminal frame, and an oversized post-terminal tail.
Proposed ordering
- const rewrittenBody = payloadRewrites.length > 0
- ? relaySseWithPayloadRewrite(nativeBody, composeSsePayloadRewrites(...payloadRewrites), translatorBudget)
- : nativeBody;
- const clientBody = relaySseWithFailedTail(rewrittenBody, upstream, reason => clientGone.abort(reason));
+ const terminalBody = relaySseWithFailedTail(nativeBody, upstream, reason => clientGone.abort(reason));
+ const clientBody = payloadRewrites.length > 0
+ ? relaySseWithPayloadRewrite(terminalBody, composeSsePayloadRewrites(...payloadRewrites), translatorBudget)
+ : terminalBody;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Windows was handled by the eager terminal-aware branch above. Remaining | |
| // tee traffic can use the JS relay to close on a protocol terminal and to | |
| // convert a mid-stream reset into a clean response.failed event. | |
| const rewrittenBody = payloadRewrites.length > 0 | |
| ? relaySseWithPayloadRewrite(nativeBody, composeSsePayloadRewrites(...payloadRewrites), translatorBudget) | |
| : nativeBody; | |
| const clientBody = process.platform === "win32" && !needsClientRewrite | |
| ? nativeBody | |
| : relaySseWithFailedTail(rewrittenBody, upstream, reason => clientGone.abort(reason)); | |
| const clientBody = relaySseWithFailedTail(rewrittenBody, upstream, reason => clientGone.abort(reason)); | |
| // Windows was handled by the eager terminal-aware branch above. Remaining | |
| // tee traffic can use the JS relay to close on a protocol terminal and to | |
| // convert a mid-stream reset into a clean response.failed event. | |
| const terminalBody = relaySseWithFailedTail(nativeBody, upstream, reason => clientGone.abort(reason)); | |
| const clientBody = payloadRewrites.length > 0 | |
| ? relaySseWithPayloadRewrite(terminalBody, composeSsePayloadRewrites(...payloadRewrites), translatorBudget) | |
| : terminalBody; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/server/responses/core.ts` around lines 2035 - 2041, Update the response
stream composition around relaySseWithPayloadRewrite and relaySseWithFailedTail
so nativeBody is first passed through relaySseWithFailedTail, then the
terminal-bounded stream is passed to relaySseWithPayloadRewrite when payload
rewrites exist. Preserve nativeBody directly when no rewrites are configured,
and add a regression test covering a payload rewrite with a coalesced
response.completed frame followed by an oversized tail.
Source: Path instructions
Stack
3/3 — carried contributor bug fixes
Base:
codex/908-long-context-pricing(#952)Summary
Carries six contributor bug fixes onto the stack, unmodified, with original authorship preserved via
git cherry-pick -x. Every commit keeps its author;git log --format='%an'shows the contributor, not me.tool_resultdocument blocksMALFORMED_FUNCTION_CALLwithout a call partredactedContentreasoning blobEight commits, +604/-70 across 20 files. No content changes were made to any of them — this is a carry, not a rework. Each applied to
devcleanly with no conflict resolution, so what is reviewed here is byte-identical to what the authors wrote.Why carry rather than merge in place
These are small, focused, independently-correct fixes that were sitting unmerged behind a long review queue. Stacking them puts them on one branch with one CI run and one review pass, in the same bottom-up order as the rest of the stack.
The source PRs stay open until this lands. If a maintainer prefers to take any of them directly instead, that path is unaffected — drop the corresponding commits here and merge the original. Once this merges, each source PR can be closed as carried, with credit already recorded in the commit history rather than in a comment.
Not carried, and why
enforce-targetfailingCHANGES_REQUESTED— carrying a PR past requested changes would route around the reviewVerification
bun x tsc --noEmit— exit 0bun teston all 11 touched test files — 331 pass, 0 failbun run test— 7691 pass, 8 skip, 0 fail, 507 filesbun run privacy:scan— passedFull-suite green is what matters here: these six fixes touch Claude inbound/outbound, Google streaming, compaction, passthrough terminals, and the Kiro adapter, and nothing in the suite regressed.
Summary by CodeRabbit
New Features
Bug Fixes
[DONE]handling, and cleanup of trailing data.Documentation